put REPLACEME we have a macro that fills it it

ditto

We probably don't want to crash when the user does socket.setRecvBufferSize(Infinity). Throwing a TypeError should be a better choice.

@TimothyGu you mean on the JS side, right?

Yes.

Noted, do you recommend:

  if (!args[0]->IsUint32()) {
    return env->ThrowTypeError("size must be an uint32");
  }

Recommend doing the type check in JS. Just something simple like:

const errors = require('internal/errors');

// then within the the functions
if (typeof size !== 'number')
  throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'size', 'number');

Thanks for your input. Do you think I need to do any safety checking in udp_wrap.cc SetRecvBufferSize() or we assume only dgram.js will interact with udp_wrap ?

Yes, once the type checking is added to the js side, I would keep the CHECK in place on the native side. That should only be hit if we do something wrong in core or if the user is doing something they really shouldn't.

Uint32Value without v8::Context specified is deprecated. Use args[0]->Uint32Value(info.GetIsolate()->GetCurrentContext()).

I think we generally prefer camelCase for JavaScript and snake case for C++.

Yes, will see to this.

Use args[0]->Uint32Value(info.GetIsolate()->GetCurrentContext()).

That’s not necessary if you already checked args[0]->IsUint32(),args[0].As<Uint32>()->Value() should do instead.

Would prefer using internal/errors here since this introduces new errors. The errno can be added.

@DamienOReilly add the new error code at https://github.com/nodejs/node/blob/master/lib/internal/errors.js#L164 (you can put placeholders in the message like in https://github.com/nodejs/node/blob/master/lib/internal/errors.js#L121

Because _handle.setSendBufferSize() includes a CHECK that will abort if size is not UInt32, these should do a proper type check in the js side. Otherwise users will not be happy.

I think we'd prefer a check in JS that will throw an internal/errors.TypeError
this is a good check Number.isFinite(num) && num <= Number.MAX_SAFE_INTEGER && num >= 0;
Ref: https://github.com/nodejs/node/blob/master/lib/internal/process.js#L31
Ref: https://github.com/nodejs/node/blob/master/lib/internal/process.js#L62

this is a good check Number.isFinite(num) && num <= Number.MAX_SAFE_INTEGER && num >= 0;

It’s not an uint32 check, though. I think num >>> 0 === num would work.

Wow, completely forgot about the existence of >>>
So much fun working with talented people 🕺

Would it make sense to also include these as options when the Socket is created?

Yes this is a good idea.

I think we generally prefer camelCase for JavaScript and snake case for C++.

Style nit: Core prefers no curly braces on single line ifs.

You shouldn't change the version this was added. Each function does support it's own changelog though. See the child_process.spawn() docs for example.

Perfect, thanks.

This function seems like a bit of overkill for detecting an unsigned int.

I think the last condition implies all the others, so it is sufficient.

If we're adding more options, it might be better to just make sure options is an object. Adding these underscored properties also means we have to maintain them indefinitely.

Technically, we could use the libuv calls to with value === 0 to get the current value.

Also, it looks like these underscored properties aren't used in setRecvBufferSize() and setSendBufferSize().

@DamienOReilly tl;dr, if you don't need them later, no need to store them.

They are used in startListening() which is called after the socket has been bound. They will in turn determine if setRecvBufferSize() and setSendBufferSize() are called if the specific options have been set against the Socket created with dgram.createSocket(). Otherwise setRecvBufferSize() and setSendBufferSize() can be called on the Socket object.

If we have to store these, I'd say use symbols. It's kind of ugly though because if the values are updated later, these values aren't kept in sync. And, libuv provides APIs to get those values.

@DamienOReilly example Symbol use https://github.com/DamienOReilly/node/blob/master/lib/net.js#L176

@cjihrig I guess extending this PR to add functionality to the interface in JS layer to use getsockopt down in libuv might be a good idea (e.g. getRecvBufferSize()). Therefore the symbol values holding initial options can be ignored after they are set against the socket when the listening event triggers.

Any reason these ifs can't be combined with the other ones?

The buffer sizes can only be set after the socket has been created.

Ah, right, sorry. I was reading the diff as if the changes were in the same function. Might be a good case for #5496 though.

No problem. Might look at that next!

Can you remove these extra blank lines.

@DamienOReilly tl;dr, if you don't need them later, no need to store them.

INHO this should be a regular error.Error (same signature)

We can just inline this in the only place it's used.

Sure thing. It makes sense.

Just a thought, but we might not need to mention SO_RCVBUF and SO_SNDBUF at all. They aren't really needed to work with the API.

They are not needed, but I though it was no harm having them mentioned in the docs. I'd like to get a few people's opinion on this.

I see that googling SO_RCVBUF gives very good results (linux man and MSDN) so I'm +0.5

You can always add something like (see socket(7)) if you like, the doctool will expand that into a link.

Just for the record, I'm not opposed to including them. I just thought we might be able to get by without them. I'm open to whatever everyone thinks is best here.

Oh I know, the only reason I included them was that I was familiar with setting the buffer sizes with the JDK, and the javadocs mentioned these property names. When I started working on a small app to get more familiar with javascript, I was goggle'in around "node.js SO_RCVBUF" to see if anyone had already added or was in the process of adding buffer size setting support in node.js.

You don't really need to initialize this to zero. It will always be set by one branch of the following if statement.

Can you line up &size with reinterpret_cast.

Same comment here about alignment.

Maybe drop syscall and just duplicate the env->ThrowUVException() call.

Hehe yes, I originally had it this way but the duplication itched at me!

I'm a little surprised the linter doesn't complain about this. Can you move type to the next line with two spaces indentation.

The duplication here could probably be reduced.

Shouldn't this be a TypeError?

Yes it should, I am throwing TypeError in dgram.js, however the test is passing. Regardless I will update the test.

common.expectsError() is subtly broken. It relies on instanceof to check the error type. So a TypeError is an instance of Error, which is why this passed. The simplest fix might be to check error.constructor inside of common.expectsError(). But that will require updating a bunch of tests because we're now using custom errors all over the place.

cc: @nodejs/testing

common.expectsError() is subtly broken

Ref: #13682

Same comment about duplication here.

I see that googling SO_RCVBUF gives very good results (linux man and MSDN) so I'm +0.5